-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build out Hyrax::ResourceForm to (mostly) support create views #4307
Conversation
75466ce
to
7d193e8
Compare
let(:options_presenter) { double(select_options: []) } | ||
|
||
before do | ||
# mock the admin set options presenter to avoid hitting Solr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An ovation for removing a significant chunk of allow
type mocking!
context 'with an existing object' do | ||
let(:work) { FactoryBot.valkyrie_create(:monograph) } | ||
|
||
xit 'renders a form' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this expected? Maybe a comment as to why it's a xit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's some discussion on this in the commit message. passing this test requires grappling with #version
/optimistic locking, which i'm considering out of scope here.
the failing tests are because of |
ee52537
to
bd16279
Compare
tests existing behavior on the controller side for the edit form. view tests are still needed, and are the next step. probably, there are behavioral problems there.
adds valkyrie work and ChangeSet/Hyrax::ResourceForm support for the `embargo/lease_enforced?` helper methods. this generalizes certain view partials to support a new form style.
since the new valkyrie models don't have pass-through attributes for embargo/lease settings, this form uses Reform's virtual properties to accept this input from the existing views. when saving the form, it will be necessary to interpret the virtual property states to determine appropriate changes to embargoes and leases. the values are prepopulated appropriately when `#prepopulate!` is called. for `#agreeement_accepted` we use a similar approach, setting to `false` by default, and to `true` when prepopulating with an existing model object.
bd16279
to
700c160
Compare
700c160
to
25baa5d
Compare
👍 the specs are mad, and Jeremy's comments are spot on, but over all this is a solid next step. Great work Tom |
c755640
to
e7eadb8
Compare
adds support for permissions to change set style forms for use with valkyrie objects.
recent work has extended the ChangeSet style `Hyrax::ResourceForm` to support most of the view behavior for work create forms. this adds tests of the relevant form partials with the new form object. the partial that handles collection membership is stubbed, since its behavior is more complex and reproducing it in the new form should be considered separately. likewise, tests for these partials as an edit form (for existing objects) are left out; mainly because versions/optimistic locking becomes an issue in this case. there's more groundwork to lay there. to support this, a registration step is added to the new model generator. this sets up the routes when we bulid the test app.
Ensures `GenericWork` remains the "primary concern" for Hyrax, by keeping it at the top of the registry. We should really consider a better approach for determining the "primary concern", and also reducing the amount code that depends on this dubious concept.
`FilterByType` used to rely on `.to_rdf_representation`, which is `ActiveFedora` specific. the default implementation is just `.name` ("RDF representation" means here "unique name referring to the model type", Class.name fits this bill fine). Add support for Valkyrie works in this context by using `.name` as a fallback. possibly we'll want a different name for *actual* RDF representations, but since this is really just for the solr index, probably we can just use `.name` forever for the new models.
extends `EmbargoHelper#embargo_enforced?` to handle the case when a form object is explictly checked for an enforced embargo, instead of its model. rather than counting on the presence of a delegated method, we call recursively with `resource.model`. since `#model` is core to the `HydraEditor::Form` interface, this should be very resiliant.
e7eadb8
to
a609e40
Compare
Extends Valkyrie forms to handle visibility, lease/embargo, and permissions (called "sharing" by the relevant form partials).
Ignored here are:
class MonographForm < Hyrax::ResourceForm(Monograph); end
All of these are future work that could benefit from ongoing discussion.
@samvera/hyrax-code-reviewers